Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactoring for 4.2 and above: Use moodle checkboxes #80

Merged
merged 2 commits into from
Oct 24, 2023
Merged

Refactoring for 4.2 and above: Use moodle checkboxes #80

merged 2 commits into from
Oct 24, 2023

Conversation

PhMemmel
Copy link
Collaborator

Hi @Syxton,

as follow-up to your comment in #71, here the PR :)

This PR reworks block_massaction to use the newly introduced bulk editing feature of moodle 4.2 and above.

CARE: This PR is not downwards compatible to moodle versions before 4.2. It would have caused a lot of overhead to make it backwards compatible, so I decided it's not worth. Especially with the new bulk editing feature we probably want to change the features of block_massaction anyway, so I think it's a good idea to handle it like that:

Suggestion:

  • Push the current (before this patch) master branch as new branch MOODLE_401_STABLE.
  • After merging the master branch will only compatible with moodle versions 4.2 and above.
  • Feel free to add a version bump commit so we can keep the versioning like we used to.
  • We can decide if future patches/features should be backported or not.

Additional information:

  • I also went over all the behat tests and updated them. Now all of them should pass (again), also locally when all supported course format plugins are installed. I also checked which formats are using indentation (again) and adapted the tests accordingly.
  • As this branch will be only compatible for moodle 4.2 and above I also adapted Github Actions configuration
  • Note for format_tiles: Currently the bulk editing feature of format_tiles is broken: the sticky footer does not appear. Luckily, for block_massaction that's not too bad, because we only rely on the checkboxes which are being added to the page despite the bug. To be able to leave bulk editing mode the user currently has to reload the page.

@PhMemmel PhMemmel self-assigned this Aug 27, 2023
@Syxton
Copy link
Owner

Syxton commented Sep 13, 2023

@PhMemmel I promise this is on my todo list, but we have had a very busy fall. Our institute moved from 3.11 to 4.2 and the faculty aren't taking it great.

@PhMemmel
Copy link
Collaborator Author

The assignment was not meant to be a ping or a reminder, just realized I did not set it, all fine 👍😀

@jrchamp jrchamp mentioned this pull request Sep 22, 2023
Merged
@Syxton
Copy link
Owner

Syxton commented Oct 20, 2023

@PhMemmel Is there any danger you see in using the Moodle checkboxes by just enabling and showing them if the Mass Actions block is loaded? Instead of having to click the Bulk Edit link? The checkboxes already exist on the page. Just seems like an extra step that is unnecessary, and the Moodle bulk editing should still work once the Bulk Edit link is clicked. It also wouldn't show the bottom menu at all which I think could be confusing.

@PhMemmel
Copy link
Collaborator Author

Yes, unfortunately the enabling of the mass action checkboxes removes all the standard edit elements of the activities and sections in the course and also the sticky footer is there which you do not want to have. So I decided to go for this extra step. If we want to use the Moodle checkboxes, this will be necessary IMO

@Syxton
Copy link
Owner

Syxton commented Oct 20, 2023

@PhMemmel When you get time, take a look at my modifications of your changes. #96 which triggers editing mode and unhides course module tools and removes the sticky-footer.

@PhMemmel
Copy link
Collaborator Author

Thanks! Will have a look on Monday and give feedback!

@PhMemmel
Copy link
Collaborator Author

Hi @Syxton,

I fixed the failing behat tests. Do you have any more comments on the PR?

If we get this merged and be able to declare compatibility with 4.3 I'm happy to have a look at the other PRs. But I think this one should go first, especially because we need to split branches as I mentioned before.

@Syxton Syxton merged commit 876ef6b into master Oct 24, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Syxton Syxton Awaiting requested review from Syxton

Assignees

@PhMemmel PhMemmel

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@PhMemmel @Syxton